feat: Display more information in the dataset preview #138
Conversation
| <DataSetMetadataBody> | ||
| {data.dataCubeByIri.contactEmail ? ( | ||
| <Link href={`mailto:${data.dataCubeByIri.contactEmail}`}> | ||
| {data.dataCubeByIri.contactEmail} |
There was a problem hiding this comment.
In the metadata there should also be the actual name associated that we should display here (contactName).
| </DataSetMetadataTitle> | ||
| <DataSetMetadataBody> | ||
| {data.dataCubeByIri.contactEmail ? ( | ||
| <Link href={`mailto:${data.dataCubeByIri.contactEmail}`}> |
There was a problem hiding this comment.
This shouldn't be next/link (since there's no page routing involved) but the Link component from theme-ui, so the proper styles can be used.
|
@bprusinowski The landing page info should be explicitly part of the metadata. In fact, it's already parsed here: visualization-tool/app/rdf/parse.ts Line 60 in 20e5ec2 |
|
@herrstucki, I guess the links shouldn't look like this – should the style from e.g. "Download data" below a published chart be applied there?
|
|
@bprusinowski Yes, at least the color and underline styles |
| wordBreak: "break-word", | ||
| "&:hover": { | ||
| textDecoration: "underline", | ||
| }, |
There was a problem hiding this comment.
It's a bit unfortunate to have to inline those styles here no ? Shouldn't we use a textStyle or refactor into a common Link component ?
There was a problem hiding this comment.
I see that theme-ui supports variants : https://theme-ui.com/components/link/#variants.
I feel like the ease of use of the sx property does not incentivize to have common style, I find it a bit problematic.
This is a nit, not a blocker and I am interested in what you think on this.
There was a problem hiding this comment.
@ptbrowne Yes, but there are other instances where this is done inline and there would be an opportunity to clean this up more generally. As a bonus challenge, we should maybe discuss if it would be worth it to replace theme-ui with Chakra. APIs are very similar but the latter is better maintained and more feature-complete in terms of the components provided.
There was a problem hiding this comment.
Exactly, I based the implementation on the other examples from the code – but I also agree that it might be worth to have some of the common logic extracted to separate components; not sure if it shouldn't be a separate issue and PR then if we agree on that!
There was a problem hiding this comment.
Personally, I think it's preferrable not to add debt and to continuously clean things up (Boy scout rule :-)), this is why I prefer to bring those problems during the review.
I think now that the PR is already approved that we can merge, but in the future I think it would be preferable to tackle those smallish issues as part of the PR.
There was a problem hiding this comment.
As for the chakra ui, I think it would be worth to try it, for a more middle term goal. This is something we can discuss in the dev weekly, I'd be glad to know more about the state of our tech stack, if we have other places where we use theme-ui and where we'd want to use chakra.
There was a problem hiding this comment.
Actually, there's already a buttons.inline variant in the theme, if you want @bprusinowski you could try to add a links section to the theme, as described here: https://theme-ui.com/components/link
|
Thanks 🎉 |
Closes #135.
This PR adds the email & landing page URL to the dataset preview.
contactEmailattached to the cube, then the the email field will contain a mailto: href; the label will be either acontactName(if exists), otherwise it'll be thecontactEmail.landingPageattached to the cube, then the landingPage field will contain a href and a label (both equal to thelandingPage).